Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list by jethrogb · Pull Request #161334 · rust-lang/rust

jethrogb · 2026-08-18T23:11:52Z

I did not use an LLM to create a change in this PR.
I used an LLM to create a change in this PR, and I have explained below how it was used.

Replace invalid uses of references in std::sys::pal::sgx::waitqueue::unsafe_list internals with raw pointers. I tried to keep the code structure the same as much as possible. In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Switched to using UnsafePinned for that.

PR organization:

Commit 1: Main soundness fix.
Commit 2: Use pinning in the pub(crate) API for UnsafeList. This code predates pinning in Rust. I believe this change isn't strictly necessary as I believe it's valid to document pinning requirements in the unsafe methods on UnsafeList. However, I felt it's better to be explicit about this now that pinning is available in the language.
Commit 3: Move UnsafeList to a platform-agnostic location so miri can be run on the test suite. This also adds some tests.

This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The additional tests developed this way have been added in the third commit. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me.

r? @nia-e

Fixes #114581
Fixes #160603
Fixes #161060
Supersedes #160641

ds84182 · 2026-08-19T07:30:08Z

What are the advantages of this PR over #161060?

Keeping the same code structure is not an advantage as the original was already overcomplicated for what should have been a simple linked list. The "dummy node" trick comes with all of this additional baggage around pinning the list and has zero upsides. The dummy node also comes with an extra vestigial T wrapped in an Option, which then needs to be unwrapped in push and pop.

nia-e · 2026-08-19T14:22:58Z

notably, this factors out a lot of complexity into an sgx-independent module which can thus be thoroughly triaged with miri - which is what I intend to make sure happens here. I haven't gotten a chance to look into this too thoroughly yet - it's going to be a big thing to review and thus will need me to sit down for a long afternoon or so - but keeping the existing code in use which has been somewhat battle-tested is at least a slight upside.

jethrogb · 2026-08-19T17:25:07Z

I think we can all agree that this code is both tricky and very important to get right: it underpins all synchronization primitives in the standard library for the target that uses it. Any problems with synchronization can lead to hard to debug and catastrophic failures of code that relies on it.

As such, we need to be conservative with making changes. The advantages of keeping the existing construction compared to changing it at all:

The target maintainers have thoroughly reviewed and fully understand the current construction
As @nia-e says, the current construction has been battle-tested. This code is backing large scale software deployments that have been running for years without any apparent synchronization issues.
- I used an LLM to compare the codegen of mainline vs commit 1 and (besides the clear changes related to initialization) it concluded the following: the theoretical cost (&mut UnsafeList/&mut WaitVariable losing noalias) produced no visible extra reloads or reordering barriers, and the link-manipulation cores are instruction-for-instruction equivalent. This also strengthens my belief that while the existing code was unsound it hasn't lead to miscompilation.

The advantages of the current construction vs. the specific alternative in #161060:

The existing construction has a better encapsulation of concerns, where the users of UnsafeList don't have to deal with raw pointers.

nia-e · 2026-08-19T17:37:35Z

I do think in the long run there is value to switching over to a "nicer" implementation, if only for maintainability and to reduce the bus factor of the SGX target. I also think it's definitely lower priority than fixing current soundness bugs, and indeed factoring out the unsafe code so it can run on Miri is a useful first step toward allowing us to thoroughly test the other suggested impl as well ^^

jethrogb · 2026-08-19T17:40:22Z

@joboet mentioned he was working on a more generic queue replacement for several targets. I think that would be a better time for introducing any major changes - it certainly helps with the bus factor.

nia-e

I spent quite some time going over this and attempting to break the code or otherwise trigger UB in Miri, and I'm happy to say I haven't managed to. I also threw Codex at it and the best it was able to come up with was an unsoundness if ~ten successive RNG failures occur and the subsequent panic is resumed from; I'm going to say that's the one blocking thing that should be addressed, but it's a small fix (see my comment).

The code itself looks fine and appears to mostly carry over the old code, which is a bit messy but will be superseded eventually by a refactor à la #161060, so nonblocking imo. The tests are a nice addition ^^ r=me once my comment is resolved & that call is properly caught.

@bors delegate+

View changes since this review

nia-e · 2026-08-28T08:10:46Z

@@ -165,19 +216,19 @@ impl WaitQueue {
                tcs: thread::current(),
                wake: false,
            }));
-            let entry_lock = lock.lock().queue.inner.push(&mut entry);
+            let entry_lock = lock.lock_pinned().as_mut().queue().inner().push(&mut entry);
            if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) {
                rtabort!("Panic before wait on wakeup event or timeout")
            }
            usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake);


This can unwind on RNG exhaustion, so should be part of the catch_unwind block above or otherwise caught.

@nia-e do you have a stack trace for the unwind?

I'm guessing it's from https://github.com/rust-lang/rust/blob/main/library/std/src/sys/pal/sgx/abi/usercalls/mod.rs#L180. This should be addressed by making sure rtabort! is done instead of panic! (or maybe it's actually non-fatal in this case?). I'm not sure if that's appropriate for all uses of random() though so it may need some more thought.

I've got the Miri backtrace ^^ manually forcing RNG to fail gave:

stack backtrace: 0: std::panicking::panic_handler 1: core::panicking::panic_fmt 2: sys::sync::unsafe_list::tests::link_then_unwind 3: ...miri_post_link_unwind_backtrace::{closure#0} 4: FnOnce::call_once 5: AssertUnwindSafe<...>::call_once 6: panicking::catch_unwind::do_call 7: panicking::catch_unwind 8: panic::catch_unwind 9: ...miri_post_link_unwind_backtrace

which suggests the callstack:

std::sys::random::sgx::fail std::sys::random::sgx::rdrand64 std::sys::random::sgx::fill_bytes <SystemRng as Rng>::fill_bytes <RangeFull as Distribution<i64>>::sample std::random::random::<i64> sgx::abi::usercalls::wait sgx::abi::usercalls::wait_timeout::wait_checked sgx::abi::usercalls::wait_timeout sgx::waitqueue::WaitQueue::wait_timeout sgx::Condvar::wait_timeout std::sync::Condvar::wait_timeout

I'm quite impressed this was able to trigger UB, but apparently it is. This is a minified form of the example Codex came up with for running in Miri (again, assuming rng always fails):

#[inline(never)] fn link_then_unwind(list: Pin<&mut UnsafeList<u32>>) { let mut entry = UnsafeListEntry::new(1234); unsafe { list.push(&mut entry) }; panic!(); } #[test] fn miri_post_link_unwind_backtrace() { let mut list = new_list(); let unwind = catch_unwind(AssertUnwindSafe(|| link_then_unwind(list.as_mut()))); assert!(unwind.is_err()); let _ = unsafe { list.as_mut().pop() }; }

I'm quite impressed this was able to trigger UB, but apparently it is.

It's a trivial “use-after-free” of stack memory. See the safety invariant on push which calls this out.

rust-bors · 2026-08-28T08:12:41Z

✌️ @jethrogb, you can now approve this pull request!

If @nia-e told you to "r=me" after making some further change, then please make that change and post @bors r=nia-e.

View changes since this delegation.

rustbot · 2026-09-07T16:04:36Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

jethrogb · 2026-09-07T16:05:21Z

Pushed two commits to deal with the randomness generation failure issue. Will run the SGX test suite before r+ing.

…c location and add more unit tests.

jethrogb · 2026-09-08T08:25:04Z

@raoulstrackx or @tvsfx could you review the two most recent commits?

Randomness generation failure is an abnormal circumstance that should lead to program termination. It's not reasonable to let consumers of `std` functionality catch such failures and resume from the.

jethrogb · 2026-09-08T13:07:07Z

SGX test suite passed on commit 998113d (job rustc-rust-sgx-ci no. 491)

tvsfx · 2026-09-10T11:38:06Z

@raoulstrackx or @tvsfx could you review the two most recent commits?

Those 2 commits LGTM

jethrogb · 2026-09-10T12:47:57Z

@bors r=nia-e

rust-bors · 2026-09-10T12:48:00Z

📌 Commit 998113d has been approved by nia-e

It is now in the queue for this repository.

@nia-e

Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list Replace invalid uses of references in `std::sys::pal::sgx::waitqueue::unsafe_list` internals with raw pointers. I tried to keep the code structure the same as much as possible. In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Switched to using UnsafePinned for that. PR organization: * Commit 1: Main soundness fix. * Commit 2: Use pinning in the `pub(crate)` API for UnsafeList. This code predates pinning in Rust. I believe this change isn't strictly necessary as I believe it's valid to document pinning requirements in the unsafe methods on UnsafeList. However, I felt it's better to be explicit about this now that pinning is available in the language. * Commit 3: Move UnsafeList to a platform-agnostic location so miri can be run on the test suite. This also adds some tests. This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The additional tests developed this way have been added in the third commit. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me. r? @nia-e Fixes rust-lang#114581 Fixes rust-lang#160603 Fixes rust-lang#161060 Supersedes rust-lang#160641

@nia-e

Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list Replace invalid uses of references in `std::sys::pal::sgx::waitqueue::unsafe_list` internals with raw pointers. I tried to keep the code structure the same as much as possible. In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Switched to using UnsafePinned for that. PR organization: * Commit 1: Main soundness fix. * Commit 2: Use pinning in the `pub(crate)` API for UnsafeList. This code predates pinning in Rust. I believe this change isn't strictly necessary as I believe it's valid to document pinning requirements in the unsafe methods on UnsafeList. However, I felt it's better to be explicit about this now that pinning is available in the language. * Commit 3: Move UnsafeList to a platform-agnostic location so miri can be run on the test suite. This also adds some tests. This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The additional tests developed this way have been added in the third commit. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me. r? @nia-e Fixes rust-lang#114581 Fixes rust-lang#160603 Fixes rust-lang#161060 Supersedes rust-lang#160641

@nia-e

Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list Replace invalid uses of references in `std::sys::pal::sgx::waitqueue::unsafe_list` internals with raw pointers. I tried to keep the code structure the same as much as possible. In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Switched to using UnsafePinned for that. PR organization: * Commit 1: Main soundness fix. * Commit 2: Use pinning in the `pub(crate)` API for UnsafeList. This code predates pinning in Rust. I believe this change isn't strictly necessary as I believe it's valid to document pinning requirements in the unsafe methods on UnsafeList. However, I felt it's better to be explicit about this now that pinning is available in the language. * Commit 3: Move UnsafeList to a platform-agnostic location so miri can be run on the test suite. This also adds some tests. This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The additional tests developed this way have been added in the third commit. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me. r? @nia-e Fixes #114581 Fixes #160603 Fixes #161060 Supersedes #160641

rust-bors · 2026-09-10T15:45:58Z

💔 Test for 9290cd7 failed: CI. Failed job:

auto - dist-aarch64-linux (web logs, enhanced plaintext logs)

rust-log-analyzer · 2026-09-10T15:57:15Z

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

JonathanBrouwer · 2026-09-10T16:24:12Z

@bors retry

@nia-e

Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list Replace invalid uses of references in `std::sys::pal::sgx::waitqueue::unsafe_list` internals with raw pointers. I tried to keep the code structure the same as much as possible. In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Switched to using UnsafePinned for that. PR organization: * Commit 1: Main soundness fix. * Commit 2: Use pinning in the `pub(crate)` API for UnsafeList. This code predates pinning in Rust. I believe this change isn't strictly necessary as I believe it's valid to document pinning requirements in the unsafe methods on UnsafeList. However, I felt it's better to be explicit about this now that pinning is available in the language. * Commit 3: Move UnsafeList to a platform-agnostic location so miri can be run on the test suite. This also adds some tests. This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The additional tests developed this way have been added in the third commit. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me. r? @nia-e Fixes rust-lang#114581 Fixes rust-lang#160603 Fixes rust-lang#161060 Supersedes rust-lang#160641

…uwer Rollup of 6 pull requests Successful merges: - #161334 (Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list) - #162346 (rustdoc: add missing CCI union logic) - #162273 (doc: replace `exceeding_bitshifts` with `arithmetic_overflow`) - #162539 (Update wasip2/wasip3 libstd crate dependencies) - #162542 (Gate ELF code in metadata.rs for ELF only) - #162604 (Update windows-gnu support docs)

@nia-e

Rollup merge of #161334 - jethrogb:fix-sgx-unsafe-list, r=nia-e Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list Replace invalid uses of references in `std::sys::pal::sgx::waitqueue::unsafe_list` internals with raw pointers. I tried to keep the code structure the same as much as possible. In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Switched to using UnsafePinned for that. PR organization: * Commit 1: Main soundness fix. * Commit 2: Use pinning in the `pub(crate)` API for UnsafeList. This code predates pinning in Rust. I believe this change isn't strictly necessary as I believe it's valid to document pinning requirements in the unsafe methods on UnsafeList. However, I felt it's better to be explicit about this now that pinning is available in the language. * Commit 3: Move UnsafeList to a platform-agnostic location so miri can be run on the test suite. This also adds some tests. This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The additional tests developed this way have been added in the third commit. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me. r? @nia-e Fixes #114581 Fixes #160603 Fixes #161060 Supersedes #160641

rustbot assigned nia-e Aug 18, 2026

rustbot added O-SGX Target: SGX S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 18, 2026

clarfonthey mentioned this pull request Aug 21, 2026

std: simplify SGX's UnsafeList #161060

Open

nia-e reviewed Aug 28, 2026

View reviewed changes

nia-e added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026

This comment has been minimized.

Sign in to view

jethrogb force-pushed the fix-sgx-unsafe-list branch from e8e4acf to 78ac762 Compare September 7, 2026 16:04

This comment has been minimized.

Sign in to view

Jethro Beekman added 3 commits September 8, 2026 10:23

Fix unsoundness issues in std::sys::pal::sgx::waitqueue::unsafe_list

82bc685

Implementing pinning in std::sys::pal::sgx::waitqueue::unsafe_list

994ae4e

Move std::sys::pal::sgx::waitqueue::unsafe_list to a platform-agnosti…

0246333

…c location and add more unit tests.

jethrogb force-pushed the fix-sgx-unsafe-list branch from 78ac762 to 37f650f Compare September 8, 2026 08:24

Jethro Beekman added 2 commits September 8, 2026 14:22

SGX: Abort instead of panic on randomness generation failure

8770fd6

Randomness generation failure is an abnormal circumstance that should lead to program termination. It's not reasonable to let consumers of `std` functionality catch such failures and resume from the.

SGX: don't abort on randomness failure in usercalls::wait

998113d

jethrogb force-pushed the fix-sgx-unsafe-list branch from 37f650f to 998113d Compare September 8, 2026 12:22

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2026

JonathanBrouwer mentioned this pull request Sep 10, 2026

Rollup of 4 pull requests #162596

Closed

JonathanBrouwer mentioned this pull request Sep 10, 2026

Rollup of 3 pull requests #162599

Closed

This comment has been minimized.

Sign in to view

rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 10, 2026

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2026

This was referenced Sep 10, 2026

Rollup of 4 pull requests #162609

Closed

Rollup of 6 pull requests #162612

Merged

rust-bors Bot merged commit 83e0995 into rust-lang:main Sep 11, 2026
13 of 14 checks passed

rustbot added this to the 1.100.0 milestone Sep 11, 2026

Uh oh!

Uh oh!

Conversation

jethrogb commented Aug 18, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ds84182 commented Aug 19, 2026

Uh oh!

nia-e commented Aug 19, 2026

Uh oh!

jethrogb commented Aug 19, 2026

Uh oh!

nia-e commented Aug 19, 2026

Uh oh!

jethrogb commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nia-e left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nia-e Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jethrogb Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jethrogb Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nia-e Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jethrogb Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

rust-bors Bot commented Aug 28, 2026

Uh oh!

This comment has been minimized.

rustbot commented Sep 7, 2026

Uh oh!

jethrogb commented Sep 7, 2026

Uh oh!

This comment has been minimized.

jethrogb commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jethrogb commented Sep 8, 2026

Uh oh!

tvsfx commented Sep 10, 2026

Uh oh!

jethrogb commented Sep 10, 2026

Uh oh!

rust-bors Bot commented Sep 10, 2026

Uh oh!

This comment has been minimized.

rust-bors Bot commented Sep 10, 2026

Uh oh!

rust-log-analyzer commented Sep 10, 2026

Uh oh!

JonathanBrouwer commented Sep 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jethrogb commented Aug 18, 2026 •

edited by rustbot

Loading

jethrogb commented Aug 19, 2026 •

edited

Loading

nia-e left a comment •

edited by rustbot

Loading

jethrogb Aug 28, 2026 •

edited

Loading

nia-e Aug 28, 2026 •

edited

Loading

jethrogb commented Sep 8, 2026 •

edited

Loading